-
-
Notifications
You must be signed in to change notification settings - Fork 54
Support Subtract and Division of NDCube by NDData #880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to wrap my head around what's happening here and I don't see anything obviously problematic (though I don't know much about NDData either). My one suggestion would be adding a test that constructs an NDData from a dask array (I think this is possible??) and then apply a these operations (at least add and mul) to that object and ensure that the laziness is preserved in these new codepaths.
|
@wtbarnes: Do these tests satisfy your comment? |
0c2e688 to
1bcd97f
Compare
f90bf15 to
9f9805f
Compare
|
@Cadair it looks like |
|
Oh, of course it doesn’t. Well balls. I'm not really sure what the best option is in that case then. |
Dare a say it? |
Do it |
…to nddataArithmetic2
| >>> import warnings | ||
| >>> with warnings.catch_warnings(): | ||
| ... warnings.simplefilter("ignore") # Catching warnings not needed but keeps docs cleaner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan, it makes the warnings surprising for people following along at home. What warnings are we squashing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember right, a warning is raised because NDUncertainty.propagate doesn't support power, and so the uncertainties get dropped.
What alternative do you suggest?
| Note that the output type of `ndcube.NDCube.to_nddata` can be controlled via the ``nddata_type`` kwarg. | ||
| For example: | ||
|
|
||
| >>> from astropy.nddata import NDDataRef | ||
| >>> nddataref2 = cube2.to_nddata(wcs=None, nddata_type=NDDataRef) | ||
| >>> print(type(nddataref2) is NDDataRef) | ||
| True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels irrelevant to the narrative here and just documentation of to_nddata, and therefore in the wrong place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. Currently, there isn't a section anywhere on to_nddata and this code snippet probably doesn't merit one? So it might as well live here unless/until there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it distracts from the narrative flow of this document, we could put it in the docstring of to_nddata as an example?
| `ndcube.NDCube.to_nddata` is not limited to changing/removing the WCS. | ||
| The value of any input supported by the ``nddata_type``'s constructor can be altered by setting a kwarg for that input, e.g.: | ||
|
|
||
| .. code-block:: python | ||
| >>> nddata_ones = cube2.to_nddata(data=np.ones(cube2.data.shape)) | ||
| >>> nddata_ones.data | ||
| array([[1., 1., 1.], | ||
| [1., 1., 1.]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as this section, feels like the wrong place for this example? It's not relevant to arithmetic operations?
Co-authored-by: Stuart Mumford <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to decide on what we are doing with the open docs comments.
To Do